-
Notifications
You must be signed in to change notification settings - Fork 42
Make L2 predeploys configurable via --l2-predeploy-json for opstack genesis #218
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds a configurable mechanism for deploying contracts at genesis via JSON files for the opstack recipe. It replaces the previous --enable-entrypoint flag with a more generic and extensible --l2-predeploy-json flag that accepts JSON file paths describing L2 predeploy accounts.
Key changes:
- Introduces a JSON-driven predeploy mechanism allowing users to inject custom contracts into L2 genesis
- Adds
PredeployAllocstruct andloadPredeployAlloc()function to parse and validate JSON predeploy configurations - Wires the existing ERC-4337 EntryPoint v0.7 JSON through this new mechanism
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
playground/utils/entrypoint_v0.7.json |
Adds JSON configuration for ERC-4337 EntryPoint v0.7 contract to be deployed at genesis |
playground/artifacts.go |
Implements PredeployAlloc type, loadPredeployAlloc() parser, and integrates predeploy JSON files into L2 genesis alloc |
playground/recipe_opstack.go |
Adds --l2-predeploy-json flag and wires it to the artifacts builder |
README.md |
Documents the new --l2-predeploy-json flag with usage examples |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
| } | ||
| if p.Address == "" { | ||
| return "", nil, fmt.Errorf("predeploy JSON missing address") | ||
| } |
Copilot
AI
Dec 1, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The address field should be validated to ensure it's a valid Ethereum address. Currently, any string can be passed and will be directly inserted into the genesis alloc, which could lead to invalid genesis configuration if an improperly formatted address is provided.
Consider adding validation:
if !gethcommon.IsHexAddress(p.Address) {
return "", nil, fmt.Errorf("predeploy JSON has invalid address format: %q", p.Address)
}| } | |
| } | |
| if !gethcommon.IsHexAddress(p.Address) { | |
| return "", nil, fmt.Errorf("predeploy JSON has invalid address format: %q", p.Address) | |
| } |
playground/artifacts.go
Outdated
| account := map[string]interface{}{ | ||
| "balance": p.Balance, | ||
| "nonce": p.Nonce, |
Copilot
AI
Dec 1, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Balance and Nonce fields are always included in the genesis alloc account, even when they're empty strings. This could potentially lead to invalid genesis data. Consider handling empty values more explicitly:
account := map[string]interface{}{}
if p.Balance != "" {
account["balance"] = p.Balance
}
if p.Nonce != "" {
account["nonce"] = p.Nonce
}Or set default values if these fields should always be present:
account := map[string]interface{}{
"balance": p.Balance,
"nonce": p.Nonce,
}
if p.Balance == "" {
account["balance"] = "0x0"
}
if p.Nonce == "" {
account["nonce"] = "0x0"
}| account := map[string]interface{}{ | |
| "balance": p.Balance, | |
| "nonce": p.Nonce, | |
| balance := p.Balance | |
| if balance == "" { | |
| balance = "0x0" | |
| } | |
| nonce := p.Nonce | |
| if nonce == "" { | |
| nonce = "0x0" | |
| } | |
| account := map[string]interface{}{ | |
| "balance": balance, | |
| "nonce": nonce, |
playground/artifacts.go
Outdated
| var p PredeployAlloc | ||
| if err := json.Unmarshal(raw, &p); err != nil { | ||
| return "", nil, fmt.Errorf("failed to unmarshal predeploy JSON: %w", err) | ||
| } | ||
| if p.Address == "" { | ||
| return "", nil, fmt.Errorf("predeploy JSON missing address") | ||
| } | ||
|
|
||
| account := map[string]interface{}{ | ||
| "balance": p.Balance, | ||
| "nonce": p.Nonce, | ||
| } | ||
| if p.Code != "" { | ||
| account["code"] = p.Code | ||
| } | ||
| if len(p.Storage) > 0 { | ||
| account["storage"] = p.Storage | ||
| } | ||
|
|
||
| return p.Address, account, nil |
Copilot
AI
Dec 1, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hex values (balance, nonce, code) from the JSON are not validated to ensure they're properly formatted hex strings. Invalid hex values would be silently passed through to the genesis alloc, which could cause issues when the genesis is processed.
Consider adding validation for hex string fields:
// After unmarshaling
if p.Balance != "" && !isValidHex(p.Balance) {
return "", nil, fmt.Errorf("predeploy JSON has invalid balance hex format: %q", p.Balance)
}
if p.Nonce != "" && !isValidHex(p.Nonce) {
return "", nil, fmt.Errorf("predeploy JSON has invalid nonce hex format: %q", p.Nonce)
}
if p.Code != "" && !isValidHex(p.Code) {
return "", nil, fmt.Errorf("predeploy JSON has invalid code hex format: %q", p.Code)
}Where isValidHex checks for "0x" prefix and valid hex characters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nicee
| if err != nil { | ||
| return nil, fmt.Errorf("failed to load L2 predeploy from %q: %w", path, err) | ||
| } | ||
|
|
Copilot
AI
Dec 1, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If multiple predeploy JSON files specify the same address, the later file will silently overwrite the earlier one without any warning. This could lead to unexpected behavior where one predeploy contract unintentionally replaces another.
Consider adding a check for duplicate addresses:
for _, path := range b.predeployJSONFiles {
data, err := os.ReadFile(path)
if err != nil {
return nil, fmt.Errorf("failed to read L2 predeploy JSON %q: %w", path, err)
}
addr, account, err := loadPredeployAlloc(data)
if err != nil {
return nil, fmt.Errorf("failed to load L2 predeploy from %q: %w", path, err)
}
if _, exists := allocs[addr]; exists {
return nil, fmt.Errorf("duplicate predeploy address %s found in %q", addr, path)
}
allocs[addr] = account
}| if _, exists := allocs[addr]; exists { | |
| return nil, fmt.Errorf("duplicate predeploy address %s found in %q", addr, path) | |
| } |
| @@ -0,0 +1,7 @@ | |||
| { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be great if we could deploy v06 - v09, and either a factory or an account to test on for each.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My thinking is users, can have there own jsons that they pass the path in via params on startup.
WDYT ?
README.md
Outdated
|
|
||
| - `--external-builder`: URL of an external builder to use (enables rollup-boost) | ||
| - `--enable-latest-fork` (int): Enables the latest fork (isthmus) at startup (0) or n blocks after genesis. | ||
| - `--l2-predeploy-json` (string[]): One or more paths to JSON files describing L2 predeploy accounts to inject into the L2 genesis (e.g. EntryPoint, paymasters, custom system contracts). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think just calling it use-predeploys or something (L2 seems irrelevant)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, main thinking was because this adds it to the genesis of the l2 config, not l1. The repo deploys both an L1 and an L2. This mechanism only affects the L2 genesis config
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But since this flag is unique for the L2 recipe, that already includes the context that this flag only runs there so it makes more sense to me to call it --predeploy-json only and remove that namespace since it is inferred.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm ok, sure!
playground/artifacts.go
Outdated
|
|
||
| // PredeployAlloc is a generic JSON schema for describing a single L2 | ||
| // predeploy account to be injected into the L2 genesis alloc. | ||
| type PredeployAlloc struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is already an alloc.GeneissAccount object in the go-ethereum repo with this data and the unmarshal/marshal methods. Ideally we can use that instead of this custom format.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahhh didn't know about that one, nice find catch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated!
| @@ -0,0 +1,9 @@ | |||
| { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This a config for launching an entry point v07 contract for 4437
|
@ferranbt Do you mind checking this again if you get a chance |
- Creating and mounting specific Docker-managed volumes for container data instead of bind mounting - Mounting validator logs to a Docker-managed volume so that we don't have removal issues on restart - Identifying Docker Compose projects by the ID in manifest at start - Taking down Docker Compose projects by the ID with automatic container and volume removal (and getting rid of removal based on the container list) - `errgroup` use for starting services on the host Fixes flashbots#181
This PR cleans a few things on the docker runtime: - It moves the application of the overrides (user can override which images/binaries use during execution) out of the runtime since they only modify the manifest ([here](https://github.com/flashbots/builder-playground/pull/254/files#diff-b7bbff9c5fe2d0c8165900e164ed7b2471fc8f3d886ccee8786e1734901ebfa4L124)). - Fixes the component unit tests. The way it was done before, the inputs were not modified since the test framework would apply the default variables. ([here](https://github.com/flashbots/builder-playground/pull/254/files#diff-35602f032417ec14bb4f3092bc1cd29cced0b7a6b1a52b108f39a68227998219R29)). - Moved the logic for the interactive mode out of the docker runtime using the callback interface from flashbots#232 ([here](https://github.com/flashbots/builder-playground/pull/254/files#diff-b7bbff9c5fe2d0c8165900e164ed7b2471fc8f3d886ccee8786e1734901ebfa4L235)). It does not make sense that the Docker runtime owns the component that deals with the visualization on the terminal.
This PR introducts a `--block-time=1s` on `builder-playground cook l1` so it's possible to product blocks faster on the launched node. I took the libery to rename the op block time flag to be unexported and to have the `InSeconds` suffix. I also used `time.Duration` flag type for the `--block-time` implementation on L1 recipe, let me know if you would like this to be applied to op block-time too, or if in the opposite, would you prefer keeping the old way for both flag.
canercidam
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Just one small request.
| return &Artifacts{Out: out}, nil | ||
| } | ||
|
|
||
| // PredeployAlloc is a generic JSON schema for describing a single L2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment seems to be hanging on an empty line. Do we need to move this somewhere else?
This PR moves some of the inlined logic in the artifact generation into some helper functions. I feel there is more we can do to modularise that part but this is already a good start.
First pass for docs
BuilderHub doesn't work without mock-proxy. Removed the optionality.
This PR fixes some static check warnings being reported by Github.
…separated ci (flashbots#271) This PR adds a new env variable `INTEGRATION_TESTS` that when set to true runs the recipe integration Go unit tests. It also introduces a new CI job that only runs those tests. Closes flashbots#268
This PR uses the correct component names for the Buildernet recipe output and adds an integration test for the BuilderNet recipe.
…lashbots#265) - New `--keep` flag for attached runs - `stop` to only stop the session services (and keep resources) - `clean` to clean up sessions - Improved listing and log inspection
Also uses warpbuild for remaining CI steps.
Co-authored-by: Chris Hager <chris@linuxuser.at>
Playground had two systems to validate the health of a service. - Functions in Go that would run on playground itself, used to determine whether the whole recipe was ready or not. - Healthcheck scripts in the docker containers managed by docker-compose to determine dependencies in the docker-compose file. Both systems evolved independently from each other, first it was 1, and 2 came about when some services had hard dependencies and would not start without the others running. But, in insight they implement the same functions, i.e. to check whether an EL node is generating blocks in sequence and at correct block time intervals. This PR eliminates 1 and integrates it inside 2.
… output folder (flashbots#263) So we have this weird indirection in which we have the artifacts builder which generates the output folder, generates the artifacts and then returns an `Artifact` struct with that output folder to be used elsewhere (i.e. other stages that might require creating artifacts). To me, it feels weird that it is the artifact builder the one that owns and creates that output folder. Instead, this PR has the artifact builder to take a reference to the output folder. Note how the tests and the main command have been simplified and look much nicer with this structure. This PR builds on top of flashbots#262
…224/builder-playground into feat/entrypointDeployment
Description 👨💻 ;
This PR adds support for passing in JSON files that define contracts you want predeployed at genesis. For the opstack recipe, we sometimes need to inject contract bytecode + metadata directly into the L2 alloc, and this gives us a simple, configurable way to do that.
We now have a generic JSON-driven predeploy flow, and the existing EntryPoint v0.7 JSON is wired through this new path. Anyone who needs things like ERC-4337 EntryPoint, paymasters, or other system contracts can just pass in JSON paths and have them included deterministically during genesis.